@git-hulk @xiaobiaozhao @PragmaTwice @torwig @ShooterIT

@torwig @PragmaTwice The reason why I use shared_ptr of SyncMigrateContext as a field of CommandClusterX is that I have to keep the lifetime of SyncMigrateContext as long as the command object.

When the slot_migrator wake up the SyncMigrateContext with a migration result status, it has to release the pointer so that no context will "occupy" the migrator any more (see here).
However, at this time point, the WriteCB have not been triggered by the event loop yet, thus the context cannot be deconstructed yet. Using a shared pointer will bind the life cycle of the SyncMigrateContext with the CommandClusterX object, which will solve this issue.

@ellutionist Thanks for your explanation.

I have not dived into these changes deeply, but if you just want to make lifetime of SyncMigrateContext as long as the command object, you can store an instance of SyncMigrateContext in CommandClusterX directly (class CommandClusterX { SyncMigrateContext ctx_; ... }), and pass some non-owning pointer (like raw pointer, no ownership) to the migrator.

@ellutionist Thanks for your explanation.

I have not dived into these changes deeply, but if you just want to make lifetime of SyncMigrateContext as long as the command object, you can store an instance of SyncMigrateContext in CommandClusterX directly (class CommandClusterX { SyncMigrateContext ctx_; ... }), and pass some non-owning pointer (like raw pointer, no ownership) to the migrator.

I agree with that

@ellutionist Thanks for your explanation.

I have not dived into these changes deeply, but if you just want to make lifetime of SyncMigrateContext as long as the command object, you can store an instance of SyncMigrateContext in CommandClusterX directly (class CommandClusterX { SyncMigrateContext ctx_; ... }), and pass some non-owning pointer (like raw pointer, no ownership) to the migrator.

@PragmaTwice @xiaobiaozhao Thank you for the advice. This is another reasonable solution. However, there are two things to note here:

1. The CommandClusterX does not always have a SyncMigrateContext (only when migrating with a "sync" flag). Making the context a non-pointer member of the CommandClusterX object may not make good sense here. If we decide to do this, I suggest to adopt std::optional<SyncMigrateContext>.
2. Due to the concern of memory safety, I am not a fan of raw pointers. Of course, in this PR we could use it, but we may set up some potential risk for the future. Because not every maintainer could get to know that "theCommandClusterX object must live long enough to complete the migration". If some change is made to "how the command objects work" in the future, it may lead to crash.

The CommandClusterX does not always have a SyncMigrateContext (only when migrating with a "sync" flag). Making the context a non-pointer member of the CommandClusterX object may not make good sense here. If we decide to do this, I suggest to adopt std::optional.

Sure, you can use something like std::optional or std::unique_ptr.

Due to the concern of memory safety, I am not a fan of raw pointers. Of course, in this PR we could use it, but we may set up some potential risk for the future. Because not every maintainer could get to know that "theCommandClusterX object must live long enough to complete the migration". If some change is made to "how the command objects work" in the future, it may lead to crash.

I think in kvrock, we use std::shared_ptr if and only if the object need to be "shared" between multiple references, and the lifetime of this object cannot be determined statically (e.g. given an object o and two reference a and b, the lifetime of o is equal to the maximum of lifetime of a and b, which need to be determined dynamically).

So currently, just "in the future ..." seems not to be an accredited reason to me, since if this reason holds, we may need to change many raw pointer in the codebase to shared_ptr. If you just need a non-owning pointer (or called an observer/view/span, in modern C++), raw pointer is currently the available choice, since std::observer_ptr or std::optional<T&> (optional with reference type) is not in the standard.

The CommandClusterX does not always have a SyncMigrateContext (only when migrating with a "sync" flag). Making the context a non-pointer member of the CommandClusterX object may not make good sense here. If we decide to do this, I suggest to adopt std::optional.

Sure, you can use something like std::optional or std::unique_ptr.

Due to the concern of memory safety, I am not a fan of raw pointers. Of course, in this PR we could use it, but we may set up some potential risk for the future. Because not every maintainer could get to know that "theCommandClusterX object must live long enough to complete the migration". If some change is made to "how the command objects work" in the future, it may lead to crash.

I think in kvrock, we use std::shared_ptr if and only if the object need to be "shared" between multiple references, and the lifetime of this object cannot be determined statically (e.g. given an object o and two reference a and b, the lifetime of o is equal to the maximum of lifetime of a and b, which need to be determined dynamically).

So currently, just "in the future ..." seems not to be an accredited reason to me, since if this reason holds, we may need to change many raw pointer in the codebase to shared_ptr. If you just need a non-owning pointer (or called an observer/view/span, in modern C++), raw pointer is currently the available choice, since std::observer_ptr or std::optional<T&> (optional with reference type) is not in the standard.

Thank you. I got the point of the difference between dynamic and static lifetime. From this point of view the share_ptr is not proper in this case. Perhaps the raw pointer is the best solution here.

Hi @ellutionist Need to use ./x.py format to correct the code format

Hi @ellutionist Need to use ./x.py format to correct the code format

Hi @ellutionist Need to use ./x.py format to correct the code format

Done. It took me a while to adapt to the recent changes from [#1420].

Also some more go test cases were added.

The implementation generally looks good to me, left a few comments:

• The blockingLock function looks unnecessary, it's fine to use std::unique_lock<std::mutex>lock(blocking_mutex_); directly
• For the naming issue, SyncMigrateContext::(StartBlock|Wakeup) is a bit inconsistent with Migrator::(StartBlocking|WakeupBlocking|CancelBlocking). And for the function name, how about using Suspend|Resume|Cancel?
• Those functions in SyncMigrateContext can be private:
  void OnWrite(bufferevent *bev); => void onWrite(bufferevent *bev);
  void OnEvent(bufferevent *bev, int16_t events); => void onEvent(bufferevent *bev, int16_t events);
  void TimerCB(int, int16_t events); => void timerCB(int, int16_t events);

Seems need run ./x.py check golangci-lint.

Those functions in SyncMigrateContext can be private:

Currently we need to make them public for the CRTP base classes to use them.

Seems need run ./x.py check golangci-lint.

Those functions in SyncMigrateContext can be private:

Currently we need to make them public for the CRTP base classes to use them.

OK, I didn't notice this limitation.

Looks like the migration test case has a segment fault. https://github.com/apache/incubator-kvrocks/actions/runs/4951001502/jobs/8855626634?pr=1418

Looks like the migration test case has a segment fault. https://github.com/apache/incubator-kvrocks/actions/runs/4951001502/jobs/8855626634?pr=1418

I think I found the reason. When building with ASAN the pointer of context won't initialize with nullptr by default. Should explicitly assign it a default value.

Merging... Thanks for your contribution!